Fix backwards compatibility in ScalarOp hash #1616
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request fixes a backwards compatibility issue in ScalarOp hash calculation. The problem was an inconsistency between the default values used in hash and equality methods, where hash used 0 while equality used None for the output_types_preference attribute. This inconsistency caused C-caching errors when comparing old cached operations with fresh ones.
- Aligns the default value in the
__hash__method to useNoneinstead of0 - Adds a test to verify hash consistency between old and new ScalarOp instances
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pytensor/scalar/basic.py | Changes default value in __hash__ method from 0 to None |
| tests/scalar/test_basic.py | Adds test case to verify hash consistency for backwards compatibility |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
ricardoV94
force-pushed
the
scalar_hash_default_bug
branch
from
0f3a014 to
5e7a46e
Compare
| test = type(self) is type(other) and getattr( | ||
| return type(self) is type(other) and getattr( | ||
| self, "output_types_preference", None | ||
| ) == getattr(other, "output_types_preference", None) |
There was a problem hiding this comment.
Note the None default instead of 0 that was used in the hash. That was the issue
ricardoV94
force-pushed
the
scalar_hash_default_bug
branch
from
5e7a46e to
bfe92a3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1616 +/- ##
==========================================
- Coverage 81.64% 81.64% -0.01%
==========================================
Files 231 231
Lines 52998 52997 -1
Branches 9395 9395
==========================================
- Hits 43268 43267 -1
Misses 7282 7282
Partials 2448 2448
🚀 New features to boost your workflow:
|
|
Sorry my report of this issue wasn't clearer. But luckily you still tracked it down! |
|
No, it was great that you noticed. This was an ugly one to have missed |
This was detected in #1582 (comment)
It was a very subtle old inconsistency between the default value used in the hash and equality methods of ScalarOps
This fix should be a blocker for the next release.
📚 Documentation preview 📚: https://pytensor--1616.org.readthedocs.build/en/1616/